-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(cli): add --watch flag to view command #685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds a --watch (-w) option to the "view" command and implements a watch/real-time rendering mode in the ViewCommand that refreshes periodically, re-renders only on content changes, and supports graceful shutdown via SIGINT/AbortSignal. Rendering was refactored to produce string output. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant ViewCommand
participant Terminal
User->>CLI: run `view` --watch
CLI->>ViewCommand: execute(path, { watch: true })
ViewCommand->>ViewCommand: initial getDashboardOutput()/getSummaryOutput()
ViewCommand->>Terminal: render initial dashboard
loop every 2s
ViewCommand->>ViewCommand: regenerate dashboard string
alt content changed
ViewCommand->>Terminal: clear screen
ViewCommand->>Terminal: print new dashboard
else no change
Note over ViewCommand,Terminal: skip reprint
end
end
User->>ViewCommand: SIGINT / AbortSignal
ViewCommand->>Terminal: stop loop and exit gracefully
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR adds a Key areas touched:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant CLI as src/cli/index.ts (commander)
participant V as ViewCommand
participant FS as File system
participant T as Task progress utils
U->>CLI: openspec view --watch
CLI->>V: execute(targetPath, {watch:true})
loop every 2s
V->>FS: read openspec/changes + openspec/specs
V->>T: getTaskProgressForChange(...)
V-->>V: getDashboardOutput()
alt output changed
V->>CLI: stdout.write(clear + output)
else unchanged
V-->>CLI: no write
end
end
U->>CLI: Ctrl+C (SIGINT)
CLI->>V: SIGINT handler
V->>V: clearInterval + exit(0)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 2 comments
src/core/view.ts
Outdated
| process.on('SIGINT', () => { | ||
| clearInterval(interval); | ||
| console.log('\nExiting watch mode...'); | ||
| process.exit(0); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIGINT handler leaks
process.on('SIGINT', ...) is registered inside execute() every time watch mode runs, but it’s never removed. In a long-lived process (or if ViewCommand.execute() is invoked multiple times in the same Node process, e.g. tests/embedders), this accumulates listeners and can trigger MaxListenersExceededWarning / multiple exit handlers firing. Prefer process.once('SIGINT', ...) or remove the listener when clearing the interval.
| // Draft section should contain empty and no-tasks changes | ||
| expect(output).toContain('Draft Changes'); | ||
| expect(output).toContain('empty-change'); | ||
| expect(output).toContain('no-tasks-change'); | ||
| expect(allOutput).toContain('Draft Changes'); | ||
| expect(allOutput).toContain('empty-change'); | ||
| expect(allOutput).toContain('no-tasks-change'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ANSI stripping mismatch
These assertions use allOutput (raw, potentially ANSI-colored) while the new lines array is stripAnsi’d. If chalk color output is enabled in the test environment, expect(allOutput).toContain('Draft Changes') / similar can fail due to escape codes splitting the plain text. Use the stripped version (lines.join('\n') or stripAnsi(allOutput)) consistently for substring assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/view.ts`:
- Around line 29-31: The catch in the update loop (where getDashboardOutput is
called) only logs to stderr so the stale dashboard remains visible; change the
catch in src/core/view.ts to clear or overwrite the painted dashboard and print
the error prominently to stdout (use the same screen-clearing helper used
elsewhere or write \u001b[2J\u001b[0;0H to stdout) and include the error message
styled with chalk; additionally add a simple retry/failure counter (e.g.,
failedUpdateCount) inside the updater function to exit or stop retrying after N
consecutive failures to avoid spamming stderr when getDashboardOutput repeatedly
throws.
🧹 Nitpick comments (6)
src/core/view.ts (5)
39-43: SIGINT listener is never removed and could leak in tests.
process.on('SIGINT', ...)registers a permanent listener. Ifexecuteis ever called more than once (e.g., in a test harness), listeners accumulate. Consider usingprocess.once('SIGINT', ...)or storing the handler and callingprocess.removeListeneralongsideclearInterval.Proposed fix
- process.on('SIGINT', () => { + const onSigint = () => { clearInterval(interval); console.log('\nExiting watch mode...'); process.exit(0); - }); + }; + process.once('SIGINT', onSigint);
46-46:await new Promise(() => {})blocks forever — intentional but fragile.This is the standard "keep-alive" pattern for CLI watch loops, but it means
execute()never returns in watch mode. The only exit path isprocess.exit(0)inside the SIGINT handler. This makes unit-testing watch mode very difficult (no way to programmatically stop it). Consider accepting anAbortSignalor returning the interval handle for testability.
214-215:totalChangesis computed but never used.This variable is dead code. Remove it or wire it into the summary output.
Proposed fix
- const totalChanges = - changesData.draft.length + changesData.active.length + changesData.completed.length;
228-231: No-opforEachon completed changes.This loop body is empty. Either remove it or implement the intended logic (e.g., accumulating task counts from completed changes into
totalTasks/completedTasks).Proposed fix — remove the no-op loop
- changesData.completed.forEach(() => { - // Completed changes count as 100% done (we don't know exact task count) - // This is a simplification - });
53-121: Inconsistent output assembly:appendvs directoutput +=.Lines 118–119 concatenate directly to
output(skippingappend), so they don't get the trailing\nthatappendadds. Meanwhile,getSummaryOutput(Line 65) is also concatenated directly. The mixing is easy to misread and could introduce subtle double-newline or missing-newline issues during future edits. Consider usingappendconsistently, or document the intent.test/core/view.test.ts (1)
9-128: No tests cover watch mode.The watch mode path (
options.watch = true) is untestable with the current design sinceexecutenever returns. Consider adding a note/TODO, or refactoringexecuteto accept anAbortSignalso watch mode can be stopped programmatically in tests.
- Fix SIGINT listener leak by using process.once - Improve error visibility in watch mode by clearing screen and showing errors in red - Remove dead code (unused totalChanges and empty loop) - Standardize string construction for consistent output - Add AbortSignal support and unit tests for watch mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/view.ts`:
- Line 27: The code currently clears the terminal including scrollback by
writing '\x1B[3J' in the process.stdout.write calls; update both occurrences in
src/core/view.ts (the process.stdout.write(...) calls that build the clear
sequence) to remove the '\x1B[3J' token so the sequence becomes only
'\x1B[2J\x1B[H' + output, thereby clearing the visible screen and repositioning
the cursor while preserving terminal scrollback.
🧹 Nitpick comments (3)
src/core/view.ts (3)
39-39: Async callback insetIntervalcan cause overlapping executions.
setInterval(update, 2000)fires every 2 s regardless of whether the previousupdate()(which isasync) has finished. IfgetDashboardOutputever takes longer than 2 s (e.g., large project, slow disk), two invocations will race and interleaveprocess.stdout.writecalls, producing garbled output.A simple fix is to use a recursive
setTimeoutinstead:Proposed fix
- const interval = setInterval(update, 2000); + let timeout: ReturnType<typeof setTimeout>; + const scheduleNext = () => { + timeout = setTimeout(async () => { + await update(); + scheduleNext(); + }, 2000); + }; + scheduleNext();Adjust cleanup references from
clearInterval(interval)toclearTimeout(timeout)accordingly.
60-69: Never-resolving promise on Line 68 keeps the function from ever returning.When no
AbortSignalis provided,await new Promise(() => {})hangs forever — the only escape isprocess.exit(0)inside the SIGINT handler. This is acceptable for CLI entry points but makes the method impossible to use programmatically or in integration tests without anAbortSignal.Consider documenting this contract explicitly (e.g., a JSDoc note that callers must pass
signalif they need the function to return), or provide a fallback that resolves when SIGINT fires instead of callingprocess.exit:Sketch
} else { - await new Promise(() => {}); + await new Promise<void>((resolve) => { + const onSigint = () => { + clearInterval(interval); + console.log('\nExiting watch mode...'); + resolve(); + }; + process.once('SIGINT', onSigint); + }); }This would unify the exit path and let the function return cleanly without
process.exit.
41-47:process.exit(0)incleanupis a hard exit that bypasses any callers awaitingexecute.When SIGINT fires (without an
AbortSignal), the cleanup handler callsprocess.exit(0), which immediately terminates the process. Anyfinallyblocks or post-executelogic in the caller will never run. This is related to the never-resolving promise on Line 68 — if the promise were made resolvable on SIGINT (as suggested above), theprocess.exithere becomes unnecessary and cleanup becomes more composable.
| if (output !== lastOutput) { | ||
| lastOutput = output; | ||
| // Clear screen, scrollback and move cursor to home | ||
| process.stdout.write('\x1B[2J\x1B[3J\x1B[H' + output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\x1B[3J clears terminal scrollback history.
The PR description states the goal is to "preserve terminal scrolling" and avoid forcing scroll, but \x1B[3J explicitly erases the scrollback buffer. If the intent is only to clear the visible screen and reposition the cursor, drop the \x1B[3J sequence and keep only \x1B[2J\x1B[H. Same applies to Line 32.
🤖 Prompt for AI Agents
In `@src/core/view.ts` at line 27, The code currently clears the terminal
including scrollback by writing '\x1B[3J' in the process.stdout.write calls;
update both occurrences in src/core/view.ts (the process.stdout.write(...) calls
that build the clear sequence) to remove the '\x1B[3J' token so the sequence
becomes only '\x1B[2J\x1B[H' + output, thereby clearing the visible screen and
repositioning the cursor while preserving terminal scrollback.
Summary
This PR introduces a
--watch(-w) flag to theviewcommand, significantly improving the Developer Experience (DX) by allowing real-time monitoring of specifications and changes without manual refreshes.Problem
Currently, users must repeatedly execute
openspec viewto check the status of their specs or track progress on changes. This breaks flow and makes it difficult to visualize updates as they happen.Solution
I implemented a robust watch mode that refreshes the dashboard automatically. Key implementation details include:
viewcommand are strictly preserved.SIGINT(Ctrl+C) correctly, ensuring the cursor is restored and the process exits cleanly.Test Plan
test/core/view.test.tsto verify the watch logic and ensure the draft/active/completed sorting remains deterministic.npm run dev:cli -- view --watchSummary by CodeRabbit
New Features
Tests